ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone #12240

dragosmg · 2022-01-24T16:03:01Z

No description provided.

github-actions · 2022-01-24T16:03:23Z

https://issues.apache.org/jira/browse/ARROW-14442

github-actions · 2022-01-24T16:03:24Z

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

r/tests/testthat/test-Array.R

r/R/array.R

dragosmg · 2022-01-26T14:55:19Z

I think this is almost ready to go. 2 questions remain in my mind:

do we want to do anything special for the cases when, for some reason, Sys.timezone() returns NA or an empty string?
do we want to inform the user about the change in timezone - from timezoneless to system time zone?

jonkeane

Thanks for all the digging on this. I have a few comments about adding a few more tests.

I also wonder if we should actually be making this change at

arrow/r/src/type_infer.cpp

Lines 73 to 77 in 01855c7

    
           auto tzone_sexp = Rf_getAttrib(x, symbols::tzone); 
        
           if (Rf_isNull(tzone_sexp)) { 
        
             return timestamp(TimeUnit::MICRO); 
        
           } else { 
        
             return timestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0)));

which already handles some of the timezone conversion, instead of inside of Array$create(). Doing it in type_infer would mean anything that uses that code would benefit and not just be limited to Array creation

r/tests/testthat/test-Array.R

jonkeane · 2022-01-26T21:06:18Z

Any thoughts about what I suggested in #12240 (review) ?

dragosmg · 2022-01-26T21:22:53Z

I think it would make a lot of sense to make the change at a lower level. In fact, that was a question I had on my list but never asked. I will look into the cpp code to see if I can figure out how to make the change there. Thanks

r/src/type_infer.cpp

dragosmg · 2022-02-01T19:37:24Z

It seems some unit tests are failing on Windows due to the timezone database not being found.

jonkeane · 2022-02-01T21:31:50Z

I'm not totally surprised that windows + timezones has popped up here. Can you tell the nature of the problem?

arrow/r/tests/testthat/test-dplyr-funcs-datetime.R

Lines 38 to 42 in 56e270f

    
           # test_date + 1 turns the tzone = "" to NULL, which is functionally equivalent 
        
           # so we can run some tests on Windows, but this skirts around 
        
           # https://issues.apache.org/jira/browse/ARROW-13588 
        
           # That issue is tough because in C++, "" is the "no timezone" value 
        
           # due to static typing, so we can't distinguish a literal "" from NULL

might be relevant here

dragosmg · 2022-02-02T15:27:56Z

@jonkeane could the solution be to simply skip the failing tests on Windows?

jonkeane · 2022-02-03T13:58:48Z

@jonkeane could the solution be to simply skip the failing tests on Windows?

We've done that for others like this. I would recommend going through the errors and confirming that they stem from https://issues.apache.org/jira/browse/ARROW-13168 and mark that as the reason for skipping on windows. Some of the error output in CI looks like it is, but others looked slightly different (they might have the same root cause still, but when I looked through the logs I was not 100% confident they were all ARROW-13168)

r/tests/testthat/test-dplyr-funcs-datetime.R

dragosmg · 2022-02-23T10:19:10Z

Currently blocked by ARROW-13168.

dragosmg · 2022-03-30T14:04:08Z

@jonkeane I revived this PR. It looks like (most of) the Windows tzdb issues have magically gone away.

rok · 2022-03-30T14:12:41Z

Yup, here's the magic: #12536

dragosmg · 2022-03-30T14:15:22Z

Yep, I know. I was waiting for that PR to get merged. 😉

r/tests/testthat/helper-skip.R

jonkeane

Woo, I like simple changes like this that just work in the end 😄

A few questions | comments

r/tests/testthat/helper-skip.R

r/tests/testthat/test-Array.R

jonkeane

This is great, a few suggestions + one request for one more test (or point to where it exists elsewhere...)

r/tests/testthat/test-Array.R

…"") ...)`

r/R/arrow-package.R

…with_timezone("")`

AlenkaF · 2022-04-05T11:16:26Z

Looks good to me 👍

ursabot · 2022-04-11T20:01:20Z

Benchmark runs are scheduled for baseline = e453ffe and contender = 633687c. 633687c is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Failed ⬇️1.0% ⬆️0.25%] test-mac-arm
[Failed ⬇️0.0% ⬆️0.36%] ursa-i9-9960x
[Finished ⬇️0.17% ⬆️0.04%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/484| 633687c1 ec2-t3-xlarge-us-east-2>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/469| 633687c1 test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/470| 633687c1 ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/479| 633687c1 ursa-thinkcentre-m75q>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/483| e453ffef ec2-t3-xlarge-us-east-2>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/468| e453ffef test-mac-arm>
[Failed] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/469| e453ffef ursa-i9-9960x>
[Finished] <https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/478| e453ffef ursa-thinkcentre-m75q>
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added the Component: R label Jan 24, 2022

jonkeane reviewed Jan 24, 2022

View reviewed changes

r/tests/testthat/test-Array.R Outdated Show resolved Hide resolved

jonkeane reviewed Jan 24, 2022

View reviewed changes

r/tests/testthat/test-Array.R Outdated Show resolved Hide resolved

dragosmg commented Jan 26, 2022

View reviewed changes

r/R/array.R Outdated Show resolved Hide resolved

dragosmg force-pushed the timestampts_missing_timezone branch from da5f7f6 to 89834d6 Compare January 26, 2022 18:05

dragosmg requested a review from jonkeane January 26, 2022 19:29

jonkeane requested changes Jan 26, 2022

View reviewed changes

r/tests/testthat/test-Array.R Outdated Show resolved Hide resolved

dragosmg requested a review from jonkeane January 26, 2022 20:56

jonkeane reviewed Jan 26, 2022

View reviewed changes

r/tests/testthat/test-Array.R Outdated Show resolved Hide resolved

jonkeane changed the title ~~ARROW-14442 [R]behaviour when converting timestamps with "" as tzone~~ ARROW-14442: [R] behaviour when converting timestamps with "" as tzone Jan 28, 2022

jonkeane changed the title ~~ARROW-14442: [R] behaviour when converting timestamps with "" as tzone~~ ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone Jan 28, 2022

dragosmg commented Feb 1, 2022

View reviewed changes

r/src/type_infer.cpp Outdated Show resolved Hide resolved

jonkeane reviewed Feb 1, 2022

View reviewed changes

r/src/type_infer.cpp Outdated Show resolved Hide resolved

jonkeane reviewed Feb 1, 2022

View reviewed changes

r/src/type_infer.cpp Outdated Show resolved Hide resolved

dragosmg force-pushed the timestampts_missing_timezone branch from 341640c to aa9f33e Compare February 2, 2022 13:50

jonkeane reviewed Feb 4, 2022

View reviewed changes

r/tests/testthat/test-dplyr-funcs-datetime.R Outdated Show resolved Hide resolved

jonkeane mentioned this pull request Feb 3, 2022

ARROW-14199 [R] bindings for format (where possible) #12319

Closed

dragosmg requested a review from jonkeane February 22, 2022 09:41

dragosmg marked this pull request as ready for review February 22, 2022 09:41

dragosmg force-pushed the timestampts_missing_timezone branch from 8708968 to c558d89 Compare February 22, 2022 10:19

added failing tests for timestamps

eaf2fe9

dragosmg added a commit to dragosmg/arrow that referenced this pull request Mar 4, 2022

added TODO to revisit the casting step once apache#12240 is merged

03eb111

dragosmg added 5 commits March 30, 2022 11:00

some cleanup

3343026

more cleanup

fa510ed

one more

d9d1d15

another one

f472103

Merge branch 'master' into timestampts_missing_timezone

c071d07

dragosmg marked this pull request as ready for review March 30, 2022 14:03

dragosmg commented Mar 30, 2022

View reviewed changes

r/tests/testthat/helper-skip.R Outdated Show resolved Hide resolved

jonkeane requested changes Mar 30, 2022

View reviewed changes

r/tests/testthat/helper-skip.R Outdated Show resolved Hide resolved

r/tests/testthat/test-Array.R Outdated Show resolved Hide resolved

r/tests/testthat/test-Array.R Outdated Show resolved Hide resolved

dragosmg added 2 commits March 31, 2022 09:53

removed the on_windows() skip helper

876c483

use withr::with_timezone()

5903fb2

dragosmg requested a review from jonkeane March 31, 2022 09:08

jonkeane requested changes Mar 31, 2022

View reviewed changes

r/tests/testthat/test-Array.R Show resolved Hide resolved

r/tests/testthat/test-Array.R Show resolved Hide resolved

r/tests/testthat/test-Array.R Show resolved Hide resolved

dragosmg added 2 commits April 1, 2022 13:00

erroring with with_timezone("" ...) instead of `with_envvar(c(TZ = …

de58c8d

…"") ...)`

add test with a different timezone

e31bf0a

jonkeane reviewed Apr 1, 2022

View reviewed changes

r/R/arrow-package.R Outdated Show resolved Hide resolved

dragosmg added 2 commits April 4, 2022 21:24

removed rlang::inform() import

e5039a8

switched back to withr::with_envvar(c(TZ = "")) instead of `withr::…

fb19ce8

…with_timezone("")`

jonkeane closed this in 633687c Apr 11, 2022

dragosmg deleted the timestampts_missing_timezone branch April 12, 2022 12:44

This was referenced Apr 11, 2022

[R] fix behaviour when converting timestamps with "" as tzone #30005

Closed

[R] Revisit binding_format_datetime and remove manual casting #31206

Open

	auto tzone_sexp = Rf_getAttrib(x, symbols::tzone);
	if (Rf_isNull(tzone_sexp)) {
	return timestamp(TimeUnit::MICRO);
	} else {
	return timestamp(TimeUnit::MICRO, CHAR(STRING_ELT(tzone_sexp, 0)));

ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone #12240

ARROW-14442: [R] fix behaviour when converting timestamps with "" as tzone #12240

Uh oh!

Conversation

dragosmg commented Jan 24, 2022

Uh oh!

github-actions bot commented Jan 24, 2022

Uh oh!

github-actions bot commented Jan 24, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragosmg commented Jan 26, 2022

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonkeane commented Jan 26, 2022

Uh oh!

dragosmg commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dragosmg commented Feb 1, 2022

Uh oh!

jonkeane commented Feb 1, 2022

Uh oh!

dragosmg commented Feb 2, 2022

Uh oh!

jonkeane commented Feb 3, 2022

Uh oh!

Uh oh!

dragosmg commented Feb 23, 2022

Uh oh!

dragosmg commented Mar 30, 2022

Uh oh!

rok commented Mar 30, 2022

Uh oh!

dragosmg commented Mar 30, 2022

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AlenkaF commented Apr 5, 2022

Uh oh!

ursabot commented Apr 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dragosmg commented Jan 26, 2022 •

edited

Loading